Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BYOM] Move from "OAI" naming convention to "BYOM" #27043

Closed
wants to merge 1 commit into from

Conversation

jonathansampson
Copy link
Contributor

With the growth of the BYOM model, Brave aims to support popular service endpoints. Originally, the OpenAI model was supported, which was reflected in relevant files and code. With the growing adoption of alternatives (e.g., Ollama locally, Anthropic, Perplexity, Azure hosted models, etc.), it is important to avoid confusion throughout the codebase. This change therefore partially renames files to reflect the move from "oai" to "byom", as well as updates classes, header files, and more. This migration lays the groundwork for a clearer, more maintainable path towards supporting even more endpoints and API patterns.

Resolves brave/brave-browser#42936

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

With the growth of the BYOM model, Brave aims to support popular service endpoints. Originally, the OpenAI model was supported, which was reflected in relevant files and code. With the growing adoption of alternatives (e.g., Ollama locally, Anthropic, Perplexity, Azure hosted models, etc.), it is important to avoid confusion throughout the codebase. This change therefore partially renames files to reflect the move from "oai" to "byom", as well as updates classes, header files, and more. This migration lays the groundwork for a clearer, more maintainable path towards supporting even more endpoints and API patterns.

Also, this change addresses the presubmit warnings associated with associated unit tests (specifically, a couple lines of JSON within).
@jonathansampson jonathansampson force-pushed the sampson-oai-to-byom-rename branch from d4d868a to 439cff3 Compare December 17, 2024 13:34
@petemill
Copy link
Member

I'm not sure what we gain by renaming these files. If we offered the user the option to switch between OAI and Anthropic API then we'd use an Anthropic API client and not the OpenAI API client right? Similarly if we expose an OpenAI endpoint for some Leo functions then we'd most likely use the existing client.
But I don't think we'd grow the OAI client files to support two different API styles.

@jonathansampson
Copy link
Contributor Author

jonathansampson commented Dec 17, 2024

I'm not sure what we gain by renaming these files. If we offered the user the option to switch between OAI and Anthropic API then we'd use an Anthropic API client and not the OpenAI API client right? Similarly if we expose an OpenAI endpoint for some Leo functions then we'd most likely use the existing client. But I don't think we'd grow the OAI client files to support two different API styles.

The OAI files are currently serving OpenAI, Ollama, Azure endpoints and more (each of which partially implement the OpenAI API spec, but with subtle differences). Rather than potentially making ongoing changes within the "OpenAI" files, I'm proposing we rename the existing files to something more generic (i.e., "BYOM" files), and then (as needed) create service-specific files for particular providers. As you suggest, this would mean the introduction of files for Anthropic, Perplexity, and more (which is what I had planned as a follow-up on this PR).

The reason I went this route (as opposed to simply creating an entirely new anthropic_api_client.cc) was to maintain a common API client with distinct methods for handling headers and responses. Rather than creating largely redundant code, I thought it would be better to instead enable us to provide provider-specific methods for constructing headers for requests, as well as handling responses. That said, I'm more than happy to take an approach that is more consistent with our current solution, and keep that logic encapsulated at the provider class level, creating distinct *_api_client.cc files, etc.

Your call :)

@jonathansampson
Copy link
Contributor Author

We have an established pattern of providing distinct API Client classes for different endpoints. While it is presently the case that our OpenAI Client handles OpenAI, Ollama, and more, it is still quite basic and straight-forward. The goal of this PR was to introduce alternative branches of logic (specific to particular providers) for constructing requests, and handling responses. It is probably wise not to deviate from the current path of keeping platform-specific logic confined to API Client classes, rather than introducing smaller platform-specific classes aimed at handling requests-construction and response-parsing alone. As such, I'm going to close this issue and focus instead on introducing additional API Clients for presently-unsupported providers.

@jonathansampson jonathansampson deleted the sampson-oai-to-byom-rename branch December 17, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BYOM] "OAI" › "BYOM"
2 participants